Add code coverage tooling (bisect_ppx)#8416
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
| (preprocess | ||
| (action | ||
| (run %{bin:cppo} %{env:CPPO_FLAGS=} %{input-file}))) |
There was a problem hiding this comment.
Dune was unabled to have both this preprocess stanza and the ppx stanza. There were such few files actually using cppo flags that I just converted them into per file rules.
ab311b7 to
8c2b726
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4f755aa31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @$(foreach bin,$(COMPILER_BIN_NAMES), \ | ||
| cp $(DUNE_BIN_DIR)/$(bin)$(PLATFORM_EXE_EXT) $(BIN_DIR)/$(bin).exe && \ | ||
| chmod 755 $(BIN_DIR)/$(bin).exe;) |
There was a problem hiding this comment.
Restore non-instrumented binaries after coverage build
coverage-build copies instrumented compiler executables into packages/@rescript/<platform>/bin, but nothing switches them back afterward. Because compiler/lib are stamp-based (_build/log, runtime build stamp), subsequent make test/make lib runs can reuse these instrumented binaries without rebuilding, so normal workflows keep running with coverage instrumentation (extra overhead and stray bisect*.coverage output) until a manual clean/rebuild. Keep the instrumented artifacts isolated or explicitly rebuild/copy uninstrumented binaries at the end of coverage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this point is valid, and one could just do
rm -f $(COMPILER_EXES) $(COMPILER_BUILD_STAMP)on finishing make coverage.
Then the next make/make test would automatically build, install, and use uninstrumented binaries again.
There was a problem hiding this comment.
Awesome thanks @cknitt I've added it and rebased on master.
Wires bisect_ppx instrumentation through every OCaml library and executable in the workspace (compiler, analysis, tools, ounit tests) and adds make targets for producing reports. - `make coverage` builds an instrumented toolchain, runs the full test suite via `node scripts/test.js -all`, and emits HTML + summary reports under `_coverage/`. - `make coverage-super-errors` is a focused flow: it runs only the super_errors fixtures and prints per-file coverage for the type checker / error reporting modules so it's easy to spot which error paths the snapshot fixtures don't exercise. - Frozen (`parsetree0.ml`) and cppo-generated `compiler/ext` variants are filtered from reports via `--ignore-files` patterns. - bisect_ppx is added as a `with-dev-setup` dep in `rescript.opam.template`, so default test builds are unaffected.
The four libraries that use cppo (compiler/{common,ml,core,ext}) had a
library-wide `(preprocess (action (run cppo ...)))` stanza, which dune
disallows in combination with `(instrumentation ...)`. That blocked
adding bisect_ppx coverage on the modules where most of the type
checker and error reporting lives.
Of the ~250 .ml files across these four libraries, only 10 actually
use cppo directives — all toggling the existing BROWSER and RELEASE
flags for the JSOO playground build and stripped debug logging. The
fix is mechanical:
- Rename those 10 source files to `*.cppo.ml` (or `*.cppo.mli`)
- Add a per-file `(rule (target X.ml) (deps X.cppo.ml) (action cppo))`
for each, matching the convention compiler/ext already uses for its
template files (hash_set.cppo.ml, vec.cppo.ml, etc.)
- Drop the library-wide `(preprocess ...)` stanzas
The same `%{env:CPPO_FLAGS=}` substitution is preserved in each rule,
so the dev / release / static / browser profile behavior in
`compiler/dune` is unchanged. js_reserved_map needs cppo's `-V OCAML:`
flag for its `#if OCAML_VERSION >= (5, 0, 0)` directive, which is
preserved on its rule specifically.
Side benefit: cppo only runs on the 10 files that need it instead of
every file in those libraries.
The flags I'd written for the report subcommands didn't match what bisect_ppx 2.8 actually accepts: - `--ignore-files <regex>` (used for excluding generated files) does not exist. The 2.8 API takes path globs via `--expect`/ `--do-not-expect`. Dropped the regex filter for now — generated files don't add meaningful noise to the report. We can revisit excluding `compiler/ext/*_string.ml` etc. with the path-based API later if it becomes useful. - `--ignore-missing-files` is only accepted by `html` and `cobertura`, not by `summary`. Removed it from summary invocations. - `cobertura` takes the output file as a positional argument, not via `-o`. Fixed. Also pulls in the regenerated rescript.opam (`bisect_ppx` was added as a `with-dev-setup` dep in rescript.opam.template earlier; dune regenerates the .opam file on build). `make coverage-super-errors` now runs end-to-end and produces a per-file table for the type-checker / error-reporting modules.
ocamlformat was failing on the renamed .cppo.ml/.mli files because the ignore list still referenced their old paths. Replaced the explicit file list with `**/*.cppo.ml` and `**/*.cppo.mli` globs, which covers the renamed files plus the existing `compiler/ext/*.cppo.ml` templates that were already listed individually. Future cppo files added under the new convention are covered automatically. Also includes the auto-promoted dune format applied to the js_reserved_map rule in compiler/ext/dune (`dune build @fmt` reformatted the multi-arg cppo invocation onto one arg per line).
Trim the coverage surface to what's actually needed for local inspection + agent queries: - Remove `coverage-report-cobertura` — Cobertura XML is only useful for CI dashboards (Codecov/Coveralls upload). Not wiring CI for now. - Remove `coverage-super-errors` and `coverage-run-super-errors` — super_errors fixtures are already executed under `make coverage` via scripts/test.js -all (which iterates tests/build_tests/*). Querying for super_errors-relevant modules is easy with jq on coverage.json or a grep on `summary --per-file`. - Add `coveralls` JSON output to `coverage-report`. Goes to _coverage/coverage.json with the structure documented in the Makefile header so an agent / jq one-liner can consume it directly. - Inline the COVERAGE_IGNORE variable since `--ignore-missing-files` is the only flag now. User-facing surface: `make coverage`, `make clean-coverage`, plus internal phases (coverage-build, coverage-prepare, coverage-lib, coverage-run, coverage-report) for re-running individual stages.
Runs make coverage on a single Linux/ARM job and uploads the coveralls-format report to Codecov. The codecov.yml keeps checks informational and suppresses inline annotations so PR coverage feedback stays low-noise for contributors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
coverage-build copies bisect_ppx-instrumented binaries into BIN_DIR; without cleanup, subsequent make/make test runs silently reuse them. Remove the instrumented executables and compiler build stamp at the end of make coverage so the next build rebuilds uninstrumented.
f4f755a to
979e347
Compare
Add code coverage tooling (bisect_ppx)
Why
We want a coverage workflow we can lean on as we move toward larger
agent-driven PRs. The goal isn't coverage itself — it's confidence:
an agent (or reviewer) can run
make coverage, see which lines achange leaves dark, and close those gaps with new fixtures before
merging.
What
make coveragerebuilds the toolchain with bisect_ppx instrumentation,runs
scripts/test.js -all, and emits:_coverage/html/index.html— human-browsable line-level report_coverage/coverage.json— Coveralls-format JSON for agents / jq / Codecovbisect_ppx is added to
rescript.opam.templateaswith-dev-setup,so default builds and CI runs are unaffected. Opt-in:
opam install bisect_ppx && make coverage.Why the dune surgery
(instrumentation (backend bisect_ppx))is incompatible withaction-style
(preprocess (action (run cppo ...))). Four libraries —compiler/{ml,core,common,ext}— used that pattern, and they'reexactly where the type checker and error reporting live.
Only 10 of the ~250 files in those libraries actually use cppo
directives (BROWSER / RELEASE / OCAML_VERSION). I renamed them to
*.cppo.mland added per-file(rule ...)blocks invoking cppo —same convention
compiler/ext/dunealready uses for itshash_set.cppo.ml/vec.cppo.mltemplates. Library-wide(preprocess ...)stanzas dropped. Validated all four build profiles(dev / release / static / browser) produce identical cppo output to
the old setup.
Codecov integration
A dedicated
.github/workflows/coverage.ymlrunsmake coverageon asingle Linux/ARM job and uploads
_coverage/coverage.jsonto Codecov.Design points:
ci.yml. Coverage needswith-dev-setup(bisect_ppx) and instrumented builds — differentartifacts, different cache slot (
opam-env-coverage-v1-...) so itdoesn't contaminate
ci.yml'sopam-env-v8-...cache.it on every OS/arch combo would multiply CI cost for no extra
information. Linux/ARM picked to align with the existing fast lane.
cancel-in-progresson PRs sorapid pushes don't queue up stale coverage runs.
codecov.ymlkeeps PR feedback low-noise:informational: trueon both project and patch checks — coveragedeltas surface as context, never as a blocking red ✗.
threshold: 0.5%absorbs jitter from non-deterministic tests.github_checks.annotations: false— no inline "this line isn'tcovered" comments cluttering the diff view.
comment.require_changes: true— Codecov only comments whencoverage actually moves.
fail_ci_if_error: falseon the upload step — a Codecov outageshouldn't redden the PR.
Verification
make(default build),make test-syntax, browser profile build:clean
make coverage: runs end-to-end, 49.79% baseline line coveragewith no diffs after the cppo refactor
representative files